Skip to content

fix(heatmap): persist drag-select rectangle across re-renders#2189

Merged
kodiakhq[bot] merged 12 commits into
mainfrom
alex/HDX-4147-heatmap-select-persist
May 15, 2026
Merged

fix(heatmap): persist drag-select rectangle across re-renders#2189
kodiakhq[bot] merged 12 commits into
mainfrom
alex/HDX-4147-heatmap-select-persist

Conversation

@alex-fedotyev
Copy link
Copy Markdown
Contributor

@alex-fedotyev alex-fedotyev commented May 5, 2026

Summary

The dashed selection rectangle on the Event Deltas heatmap collapses to a 2x2 px box the instant the user releases the mouse. The filter itself is applied correctly (URL gets xMin/xMax/yMin/yMax, the comparison legend shows "Selection" vs "Background", and the bar charts below split into two colors), but visually the user has no idea what slice of the heatmap they picked.

image

Repro on play-clickstack: open the Demo Traces source, switch to the Event Deltas tab, drag any region. Inspect the .u-select element after mouse-up: width=2 px, height=2 px, position pinned to top-left of the canvas.

Same component is used in the Search heatmap (DBSearchHeatmapChart), so this fix applies there too. Dashboard tile heatmaps don't pass onFilter, so they're not affected.

Linked: HDX-4147.

Root cause

uplot-react destroys and recreates the chart whenever its options prop reference changes and optionsUpdateState classifies the change as 'create' (any non-width/height top-level key is not Object.is-equal). In HeatmapContainer's Heatmap:

  1. tickFormatter was a useCallback keyed on numberFormat reference. Callers (e.g. DBSearchHeatmapChart) build {output: 'duration', factor: 0.001} inline on every render, so numberFormat is a fresh ref each time.
  2. New tickFormatter ref makes the options useMemo recompute, returning a fresh object whose series, axes, cursor, and plugins are all new array/object literals.
  3. uplot-react sees a new options ref, walks the keys, classifies as 'create', destroys the old chart, builds a new one. uPlot's u.select lives on the chart instance, so it gets wiped along with the canvas.
  4. The 2x2 px residue is uPlot's default zero-size selection element with its 1 px border on each side.

The earlier [time, bucket, count] memoization stabilized the data prop but left the options recreation cycle untouched, so the symptom persisted.

Fix

Two layers, one targeted at the recreation cycle, one accepting that recreation can happen for any reason and recovering from it.

  1. Stabilize tickFormatter by hashing numberFormat with JSON.stringify instead of depending on its reference. Same content -> same hash -> same tickFormatter -> same options -> no recreate. The hashed-content pattern is correct here because the formatter only reads top-level scalar fields off numberFormat.

  2. Treat the URL as the source of truth for the selection rectangle. DBSearchHeatmapChart already writes xMin/xMax/yMin/yMax to the URL via nuqs. I plumb those back down through a new selectionBounds prop on DBHeatmapChart -> Heatmap, and reapply via setSelect from uPlot's ready hook on every chart construction plus a useEffect for in-place bounds changes. Layer 2 is what actually keeps the rectangle alive on page reload, theme switch, resize bounce, or any future cause of recreation. The ready hook is needed (rather than onCreate) because mode-2 facet data leaves u.scales.y.min/max unpopulated until the first draw lands; reading them earlier returns undefined and the apply silently no-ops.

The bottom-bucket adjustment (yMin = 0 when the drag touched the floor of a log axis) round-trips correctly: applySelectionToChart clamps Math.log(yMin) to the chart's u.scales.y.min when yMin <= 0.

Test plan

  • Bug reproduced on the Vercel preview build for this branch (drag-select on Demo Traces > Event Deltas, observed .u-select collapse to 2x2 px at the canvas origin).
  • After the fix on a fresh Vercel build:
    • Drag-select renders a persistent dashed red rectangle: inline style left: 65px; top: 69px; width: 183px; height: 133px after mouseup, and stays visible.
    • Reloading the page with xMin/xMax/yMin/yMax in the URL restores the rectangle to the same coordinates the user dragged. uPlot's inline style is set via the ready hook.
    • Clicking on the chart away from the rectangle clears both the URL state and the rectangle (inline style goes to left: 0; top: 0; width: 0; height: 0).
  • yarn workspace @hyperdx/app jest --testPathPatterns heatmap passes (1549 tests).
  • yarn workspace @hyperdx/app run tsc --noEmit introduces no new errors in this diff (the 8 pre-existing errors in KubernetesDashboardPage, ServicesDashboardPage, SessionsPage, SourceForm, SourcesList, SourceSelect are also present on main).

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 5, 2026

🦋 Changeset detected

Latest commit: 4ba3b33

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch
@hyperdx/otel-collector Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link
Copy Markdown

vercel Bot commented May 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment May 15, 2026 3:49pm

Request Review

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label May 5, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 262 production lines changed (Tier 2 max: < 250)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 2
  • Production lines changed: 262 (+ 148 in test files, excluded from tier calculation)
  • Branch: alex/HDX-4147-heatmap-select-persist
  • Author: alex-fedotyev

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

PR Review

Solid fix with tests, good in-code rationale, and the URL-as-source-of-truth approach is robust to any future cause of chart recreation. No critical issues found.

A few minor observations worth considering:

  • ℹ️ scaleTypeRef (DBHeatmapChart.tsx:1038) is likely redundant — scaleType is already in the options useMemo dep array (line 1277), so the ready hook's closure already captures the latest value. Only selectionBoundsRef is structurally required (selectionBounds is intentionally not in the deps). Harmless as defensive coding, but you could drop it to remove indirection.
  • ℹ️ The deps-less useEffect mirroring refs (DBHeatmapChart.tsx:1041) runs on every commit. Intentional and the comment says so, but useEffect(() => {...}, [selectionBounds, scaleType]) would be equivalent here and easier to reason about (refs only need to update when the mirrored values change).
  • ℹ️ JSON.stringify(numberFormat) (DBHeatmapChart.tsx:1082) — the comment correctly flags the function-valued-field footgun. Consider switching now to an explicit shallow-compare keyed on the known NumberFormat fields; it's a tiny helper and removes the silent-failure risk before the type evolves.
  • ℹ️ applySelectionToChart is exported solely for testing. Fine pattern, but the JSDoc could mention it's @internal so consumers don't lean on it.

✅ Tests cover the tricky cases (clamping, log vs linear, reversed bounds, unpopulated scales, fireHook=false). Changeset present. No security concerns.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

E2E Test Results

All tests passed • 178 passed • 3 skipped • 1222s

Status Count
✅ Passed 178
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 6, 2026

Compound Engineering Review

✅ No critical issues found. This is a well-engineered fix for the heatmap drag-select rectangle persistence bug. The imperative applySelectionToChart escape hatch is well-bounded, types are tight, and the commentary explains the load-bearing invariants (the numberFormatKey fingerprint, the clear-before-scale-guard ordering in applySelectionToChart, the seconds → ms x-axis convention). Suggestions below are optional polish.

P2 packages/app/src/components/DBHeatmapChart.tsx:80-83 — when bounds != null but u.scales.y.{min,max} are undefined, applySelectionToChart early-returns without clearing, so a stale rectangle from a prior render can persist while scales settle. Asymmetric with the null-bounds path that always clears. → either clear before the guard, or add a one-line comment that the stale-during-settle behavior is intentional and the ready hook will catch up.

P2 packages/app/src/components/DBHeatmapChart.tsx:1037-1041 — the dep-less useEffect that mirrors selectionBounds/scaleType into refs every commit lands after paint, so on a chart recreation the ready hook (which reads the refs) can briefly see stale values during the same commit. Bounds change is recovered one frame later by the bounds-effect, but it's avoidable. → assign refs inline during render (selectionBoundsRef.current = selectionBounds; scaleTypeRef.current = scaleType;), drop the effect entirely. Same semantics, no ordering question, no ESLint suppression needed.

P2 packages/app/src/components/DBHeatmapChart.tsx:1029uplotRef is set by uplot-react but never nulled on unmount. With key={JSON.stringify(config)} recreations, theoretically the bounds effect could fire against a torn-down instance during a recreate seam. → add useEffect(() => () => { uplotRef.current = null; }, []) and an if (!u.root?.isConnected) return; guard inside applySelectionToChart. Belt-and-braces; harmless today.

P2 packages/app/src/components/DBHeatmapChart.tsx:1102-1119numberFormatKey = JSON.stringify(numberFormat) works but the 13-line comment justifying it is itself a smell, and key-order divergence between callers could miss cache hits. The comment already flags the function-field risk. → consider a [output, mantissa, unit, ...].join('|') keyed on the known NumberFormat fields (confirmed plain primitives in packages/common-utils/src/types.ts). Cheaper, more robust, lets the comment shrink.

P2 packages/app/src/components/DBHeatmapChart.tsx JSDoc on applySelectionToChart (around line 38-43) — comment implies "renderer clamps to chart's visible y-axis floor" applies generally, but the floor-clamp is only the log-scale path; linear silently passes yMin through Math.max(yMin, yScaleMin) which can shrink the rectangle if yScaleMin > 0. Behavior is correct, doc is misleading. → tighten to "log scale clamps yMin=0 to axis floor; linear passes yMin literally".

P2 packages/app/src/components/DBHeatmapChart.tsx (bounds useEffect + ready hook) — on chart create with non-null bounds, both paths apply (ready then the bounds effect). Idempotent with fireHook=false, but each recreation costs two valToPos reads. → harmless; if you trim, prefer keeping ready (handles recreations) and dropping the bounds effect's mount-time apply via a useRef guard, or just leave it and add a one-line comment so the next maintainer doesn't "fix" one path.

Skipped as not actionable: the simplicity reviewer's suggestion to delete the memos on generatedTsBuckets / heatmapData — those are load-bearing for the fix (preventing uplot-react's recreate path that wipes u.select), and the author's comments correctly call this out.

alex-fedotyev pushed a commit that referenced this pull request May 6, 2026
Three small comment additions in DBHeatmapChart.tsx (compound review on
#2189 flagged these for the next reader):

- `applySelectionToChart`: note the bounds-null clear path runs BEFORE
  the y-scale-not-populated guard so a future refactor doesn't swap the
  ordering and silently suppress clears on first paint.
- The ref-mirroring `useEffect` deliberately has no deps array; add a
  trailing comment so it doesn't read as "missing deps" at a glance.
- `numberFormatKey`: document the dependency on NumberFormat being
  JSON-serializable, with a switching note in case the type ever grows
  a function-valued field.
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

P3 nits addressed in 287183d (clarifying comments only, no behavioural change):

  • Note the bounds-null clear path runs BEFORE the y-scale-not-populated guard so a future refactor doesn't swap the ordering and silently suppress clears on first paint
  • Trailing comment on the deps-less ref-mirroring useEffect so it doesn't read as "missing deps" at a glance
  • numberFormatKey: documented the JSON-serializability assumption with a note to switch to a shallow-equal helper if NumberFormat ever grows a function-valued field

alex-fedotyev and others added 8 commits May 8, 2026 00:18
The dashed selection rectangle on the Event Deltas heatmap collapses to
a 2x2 px box the instant the user releases the mouse. The filter is
applied correctly (URL params, comparison legend, bar charts all
update), but visually you can no longer see what region you picked.

Root cause: uplot-react's dataMatch deep-compares the data prop and
calls chart.setData(data, true) when refs differ. setData resets
uPlot's internal u.select rectangle. HeatmapContainer rebuilt the
[time, bucket, count] arrays in its render body, so on every parent
re-render those entries had new references with identical values, and
dataMatch returned false.

Memoize the data array with stable refs so dataMatch returns true,
setData is skipped, and u.select survives the re-render that follows
a drag (the URL param write triggers).

Also memoize generatedTsBuckets so its fresh-Date-array each render
doesn't propagate into the heatmapData useMemo deps.

HDX-4147
Lint feedback from CI:
- timestampColumn?.name dep is wrong since the closure uses the full
  timestampColumn object; pass the object instead. The ref is stable
  when data is stable, so this doesn't reintroduce the original bug.
- Stop destructuring bucket/count when only time is needed for the
  not-enough-data short-circuit.

Add changeset (patch on @hyperdx/app).
Bot review feedback: timestampColumn was a non-primitive useMemo dep
derived entirely from data?.meta. Move the call inside the useMemo so
data is the only relevant dep, which is the cleanest way to satisfy
both exhaustive-deps and the ref-stability the fix relies on.

Also drop the // x values / // y value series 1 / // y value series 2
labels — they describe what the line is rather than why.
prose-lint flags U+2014 anywhere; tighten the comment instead.
Drop the dataMatch description (it was element-wise === per series,
not top-level === as written) and consolidate to the actionable
mechanism: fresh data ref drives uplot-react to call setData(data, true)
which wipes u.select. Add HDX-4147 ticket reference.
The previous memoization fix was insufficient. The actual cause is that
uplot-react destroys and recreates the chart whenever its `options` prop
ref changes. `tickFormatter` was rebuilt on every render because
`numberFormat` is a fresh object reference from callers (e.g.
DBSearchHeatmapChart constructs `{output: 'duration', factor: 0.001}`
inline). Rebuilt tickFormatter cascaded into the `options` useMemo,
optionsUpdateState detected new top-level keys (series, axes, cursor,
plugins are new array/object literals on each recompute), classified the
change as 'create', and the chart was destroyed and recreated, wiping
`u.select`.

Two layers:

1. Stabilize `tickFormatter`'s dep on `numberFormat` content via
   JSON.stringify, so callers passing a fresh-each-render `numberFormat`
   no longer cause chart recreation.

2. Pass the URL-backed selection into `Heatmap` as a `selectionBounds`
   prop and reapply via `setSelect` on chart create + on the prop
   changing. This makes the URL the source of truth and survives any
   future cause of chart recreation (theme switch, resize bounce, etc.).
Page reload with a URL-encoded selection wasn't drawing the rectangle.
Root cause: at onCreate time, uPlot has constructed but not yet completed
its initial layout for mode-2 facet data. u.scales.y.min/max can still
be undefined, so applySelectionToChart's early-out triggered and setSelect
never got called.

Move the apply call to uPlot's `ready` hook, which fires once after the
initial draw completes. Hold selectionBounds and scaleType in refs so
the hook (captured inside the options useMemo) reads the latest values
without requiring memo dep churn.
Three small comment additions in DBHeatmapChart.tsx (compound review on
#2189 flagged these for the next reader):

- `applySelectionToChart`: note the bounds-null clear path runs BEFORE
  the y-scale-not-populated guard so a future refactor doesn't swap the
  ordering and silently suppress clears on first paint.
- The ref-mirroring `useEffect` deliberately has no deps array; add a
  trailing comment so it doesn't read as "missing deps" at a glance.
- `numberFormatKey`: document the dependency on NumberFormat being
  JSON-serializable, with a switching note in case the type ever grows
  a function-valued field.
@alex-fedotyev alex-fedotyev force-pushed the alex/HDX-4147-heatmap-select-persist branch from 287183d to 41b87cc Compare May 8, 2026 00:18
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

Deep Review

🟡 P2 -- recommended

  • packages/app/src/components/DBHeatmapChart.tsx:~1208 -- the prior // eslint-disable-next-line react-hooks/refs directive above highlightDataPlugin({...}) was removed even though mouseInsideRef.current is still read inside that call and two new .current reads (selectionBoundsRef, scaleTypeRef) were added in the ready hook a few lines below.

    • Fix: Run make ci-lint locally to confirm the rule no longer fires here, otherwise restore the disable directive with a -- ... justification.
    • kieran-typescript, maintainability, project-standards, adversarial
  • packages/app/src/components/DBHeatmapChart.test.tsx -- the new test suite only exercises the pure applySelectionToChart helper, so the actual regression (chart no longer recreating on URL-state change, the ready hook reapplying after recreation, the bounds useEffect reapplying on resize/prop change) is covered only by manual Vercel testing.

    • Fix: Add one component-level test that mounts <Heatmap>, mutates a prop that previously caused a recreate (e.g. a fresh numberFormat object with identical content), and asserts the selection rectangle survives.
    • testing, maintainability, julik-frontend-races
  • packages/app/src/components/DBHeatmapChart.tsx:1086 -- numberFormatKey relies on JSON.stringify(numberFormat) being a faithful fingerprint, but no test pins memo stability across structurally-equal-but-reference-different objects, and the documented failure mode (future function-valued field silently dropping) is unguarded.

    • Fix: Add a unit test asserting tickFormatter identity stability across equal-content numberFormat props, or extract a typed fingerprint helper that fails closed on non-serializable fields.
    • testing, maintainability, adversarial
  • packages/app/src/components/DBHeatmapChart.test.tsx -- missing edge-case coverage for applySelectionToChart: zero-width drag (xMin === xMax), reversed yMin > yMax, negative yMin under log scale, and a scale-type switch with bounds still set.

    • Fix: Add short cases mirroring the existing test style for each.
    • testing, adversarial
🔵 P3 nitpicks (10)
  • packages/app/src/components/DBHeatmapChart.tsx:1037-1044 -- scaleTypeRef is redundant because scaleType is already in the options useMemo deps, so the ready closure could read it directly; only selectionBoundsRef is load-bearing.

    • Fix: Drop scaleTypeRef and pass scaleType to applySelectionToChart from the closure, or split the comment to note that the scaleType mirror is symmetric, not required.
    • maintainability
  • packages/app/src/components/DBHeatmapChart.tsx:698,1110 -- the two new // eslint-disable-next-line react-hooks/exhaustive-deps directives omit the inline -- reason suffix that pre-existing disables in this file carry.

    • Fix: Append a brief reason to each disable explaining why the dep is intentionally narrowed.
    • project-standards
  • packages/app/src/components/DBHeatmapChart.test.tsx -- the test file sits next to the component, while the package convention (CLAUDE.md plus the existing tests under packages/app/src/components/__tests__/) is to colocate tests in __tests__/.

    • Fix: Move to packages/app/src/components/__tests__/DBHeatmapChart.test.tsx.
    • project-standards
  • packages/app/src/components/DBHeatmapChart.tsx:1067-1084 -- the 18-line comment defending JSON.stringify(numberFormat) is longer than the cleaner alternative would be (memoize the numberFormat literal in DBSearchHeatmapChart, or depend on the known-primitive fields directly).

    • Fix: Stabilize numberFormat at the upstream call site, or depend on a primitive tuple here, removing the need for the stringify hash.
    • maintainability, kieran-typescript
  • packages/app/src/components/DBHeatmapChart.tsx:1041 -- the no-deps useEffect that mirrors props into refs runs on every commit; the equivalent useEffect(() => {...}, [selectionBounds, scaleType]) is functionally identical and self-documenting.

    • Fix: Add the explicit deps array.
    • kieran-typescript
  • packages/app/src/components/DBHeatmapChart.tsx:80-84 -- when selectionBounds is non-null and u.scales.y has not been populated, the helper returns silently with no retry trigger beyond the bounds/scale/size useEffect, so if the ready hook ever fires before scales land and no subsequent prop change happens, the rectangle stays missing.

    • Fix: Hook uPlot's setScale event or schedule a single requestAnimationFrame retry so the rectangle reapplies once scales are populated.
    • julik-frontend-races, correctness
  • packages/app/src/components/DBHeatmapChart.tsx:856-869 -- after the bottom-bucket adjustment writes yMin=0 to the URL, applySelectionToChart clamps back to yScaleMin, so the redrawn rectangle in log mode is visibly taller than the user's original drag endpoint.

    • Fix: Capture the user's original yMin in URL state alongside the SQL-filter yMin, or document this widening at the wrapper's comment so it isn't read as a bug later.
    • julik-frontend-races, correctness
  • packages/app/src/components/DBHeatmapChart.tsx:65-75 -- the "clear path runs BEFORE the scale-not-populated guard" tripwire comment suggests the structure itself could be clearer.

    • Fix: Extract a tiny named clearSelection(u) helper so the ordering is self-evident from the code shape rather than from a refactor warning.
    • maintainability
  • packages/app/src/components/DBHeatmapChart.test.tsx -- most assertions verify u.valToPos call arguments instead of the resulting u.setSelect geometry, coupling tests to the helper's internal call shape rather than its observable output.

    • Fix: Prefer asserting on u.setSelect.mock.calls[0][0] (the final rectangle) where possible.
    • testing
  • packages/app/src/components/DBHeatmapChart.tsx:1244-1250 -- on a same-commit recreate that also changes bounds, the ready hook reads selectionBoundsRef.current before the parent's no-deps mirror useEffect fires (child effects run first), so the rectangle briefly draws with the previous bounds before the bounds useEffect overwrites them.

    • Fix: Consolidate to a single source of truth (drop the ready hook and rely on the bounds useEffect after uplotRef.current is set), or move the ref-mirror to a useLayoutEffect.
    • julik-frontend-races, correctness

Reviewers (7): correctness, testing, maintainability, kieran-typescript, julik-frontend-races, adversarial, project-standards.

Testing gaps:

  • No integration test for the central HDX-4147 regression (chart not recreated on URL-state change; selection survives mouseup).
  • No test for the bounds/scale/size useEffect on container resize.
  • No test for DBSearchHeatmapChart's URL-to-SelectionBounds memo, including the all-null short-circuit.
  • No test for the bottom-bucket round-trip (drag touching log-axis floor → yMin=0 URL → reapply clamped to yScaleMin).

@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

/just-fix-it

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 9, 2026

Fixed:

  • DBHeatmapChart.tsx:59 (P0) — exported applySelectionToChart; added DBHeatmapChart.test.tsx with 12 unit tests covering null/undefined clear, scale-not-populated early return, log/linear branching, yMin === 0 clamp to yScaleMin (both scales), yMax > yScaleMax clamp, xMin * 1000 ms conversion, reversed xMin > xMax, non-negative height, and fireHook=false.
  • DBHeatmapChart.tsx:1054-1058 (P2) — added width, height to the reapply effect's deps so the persisted rectangle is reprojected on container resize. Moved useElementSize above the effect to bring the values into scope.

Skipped:

  • P2 ref-mirror race (lines 1037-1056 / 1242-1248) — fix has two non-equivalent options (render-time ref assignment vs. onCreate-driven apply) with different React-strict-mode tradeoffs; ambiguous per rules. Recommend follow-up.
  • All P3 nitpicks per rules.

@alex-fedotyev alex-fedotyev requested review from a team and wrn14897 and removed request for a team May 12, 2026 02:19
@alex-fedotyev
Copy link
Copy Markdown
Contributor Author

alex-fedotyev commented May 13, 2026

Bumping for review. It's been about a week and I want to make sure this is on the queue. Happy to rebase or split if that would help move it along.

@kodiakhq kodiakhq Bot merged commit d2b6dde into main May 15, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge compound-review review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants